Note we could have this wrap AbortReason -- we had an internal NegotiationError struct that already wrapped this along with contributions -- but it would require making AbortReason public. Not sure if we want to expose all its variants.

Little unfortunate we need to do this, but I couldn't find a better way other than passing NegotiationFailureReason to abandon_quiescent_action and quiescent_action_into_error, which doesn't seem right since they could be for future QuiescentAction variants.

Does feel like quiescent_action_into_error could take the NegotiationFailureReason and we could also use NegotiationFailureReason for dual-funding (the DiscardFunding variant) but this seems fine.

Didn't feel like there was much value to these.

• abandoned_funding_txo would be set only if there's a failure after the transaction was negotiated but before signed.
• In the future, when channel_type can change, maybe it is just part of FundingContribution? Could also keep it if you prefer.

Minor: The debug_assert!(false) path is only reachable in non-production builds if a caller passes a non-FailSplice variant. Currently the only caller passes through quiescent_action_into_error which always produces FailSplice for production code (only DoNothing in test builds). But debug_assert!(false) will silently return self unchanged in release builds — consider using debug_assert! with a more specific message about which variant was unexpected, or restructuring so the method only accepts FailSplice to make this impossible.

See #4514 (comment)

The is_initiator fallback when funding_negotiation is None defaults to false, which means splice_funding_failed_for! will return None for non-initiators (since the macro has None if !$is_initiator => None). This means if the funding negotiation was already taken before reset_pending_splice_state is called and the node was actually the initiator, the splice failure event would be silently dropped.

Is funding_negotiation ever None when reset_pending_splice_state is called? If so, defaulting to false may suppress failure events for the initiator. If not, this should be a debug_assert or .expect() to catch violations of that invariant.

Added debug assertions.

Same concern here: funding_negotiation.as_ref() being None causes is_initiator to default to false, which will cause the splice_funding_failed_for! macro to return None for non-initiators. Since maybe_splice_funding_failed is the read-only version used during serialization, silently returning None means the splice failure events won't be persisted and the user won't learn about the failure after restart.

Consider whether funding_negotiation can actually be None in this path, and if not, using expect to enforce the invariant.

Added debug assertions.

nit: Unlike handle_quiescent_error (line ~6619), this site unconditionally emits DiscardFunding even when contributed_inputs and contributed_outputs are both empty (which happens for the initiator when all inputs are committed to prior rounds). The same inconsistency exists at lines ~4417, ~4920, ~11853, ~12012, ~12180, ~12329, ~15448, and ~18122.

Consider adding the same guard here:

if !splice_funding_failed.contributed_inputs.is_empty() || !splice_funding_failed.contributed_outputs.is_empty() {
    // emit DiscardFunding
}

Or, equivalently, have the macro return None for the initiator when there are no unique contributions (matching the non-initiator behavior) and emit SpliceNegotiationFailed without DiscardFunding in those cases.

Fixed by making SpliceFundingFailed consumable into Option-al parts rather than exposing fields. The into_parts method now checks for empty contributions.

Is the "Message data" in tx_abort not a string that we can include here?

Done.

Almost each docstring says "Retry by calling splice_channel". That doesn't seem like particularly helpful advice especially in cases where the counterparty rejected implying just trying again is unlikely to fix it?

Here it really depends on the reason. They may just not like our fee rate, for instance. Protocol violations result in a disconnect rather than an abort. Hmm... that makes me wonder if retrying on disconnect is always the right approach... Presumably protocol violations are an LDK bug.

It does seem like some of these could use more details (error message string?). As-is given the recommendation for ~all enum variants is "retry by calling splice_channel again" its not really clear to me why we should bother with an enum - an enum is useful if the downstream code has different handling for different cases, but here that doesn't really seem likely/possible. If we can't expose more details that are useful for automated action, we could probably do with an enum with three variants and a developer-readable string.

Yeah, the docs could be improved here. Maybe re-calling splice_channel isn't always necessary? In some cases, calling funding_contributed at a later time may be sufficient. We are providing the FundingContribution after all.

So it does seem like different actions are required depending on the variant.

• PeerDisconnected: retry upon re-connection
• ChannelNotReady: retry when channel is in a usable state (assuming it's not closing)
• FeeRateTooLow: retry with a higher fee rate
• ContributionInvalid: retry with a re-computed contribution
• NegotiationError: probably not retryable as it indicates a counterparty error
• CounterpartyAborted: might be able to retry but would need to examine the tx_abort message (e.g., counterparty doesn't like your fee rate).

We could have a top-level Retryable that holds one of these instead, if you prefer? We could go as far as only putting the FundingContribution in applicable sub-variants. Though a user may want to see it for other variants.

I assume ChannelNotReady is really "you can't retry" (why were you splicing an unopened channel anyway? but probably its closing), FeeRateTooLow and ContributionInvalid are really "retry now with a fresh splice_channel call as that'll tell you what the new required feerate is to do a fresh rbf from scracth or will fix your contribution, hopefully". So it does seem like its basically "retry with a fresh splice_channel call, or not", with the one maybe-exception of PeerDisconnected? We could leave the enum and include a function that returns "retryable or not", but I'm not really sure why we shouldn't just return a string+bool pair (in a struct) - do we anticipate downstream code will have any different behavior based on anything beyond "should I retry this" or is it just gonna log/print an error?

Probably not other than when / how should you retry in case of the PeerDisconnected.

I suppose given we don't know when the peer will reconnect, we might as well just process it the same and call splice_channel again upon re-connection. Otherwise, we'd need to make sure not to process the associated DiscardFunding which would mean locking up the UTXOs indefinitely. Instead, the client can simply always queue up a "retry when connected".

Discussed offline. Updated to have an is_retryable method that the counterparty calls rather than examining the variant. Would prevent allocating strings in many cases.

We should document which comes first and maybe test it more exhaustively - eg in get_and_clear_pending_events assert the order if both events appear for the same channel?

nit